fix: Handle geojson features that cross antimeridian in computeBbox#7891
Conversation
| * @return {[number, number, number, number]|null} `[west, south, east, north]` | ||
| * in degrees; `east` may exceed 180° when the input crosses ±180°. | ||
| * Returns `null` for input with no extractable coordinates (e.g. `Sphere`, | ||
| * empty FeatureCollection). |
There was a problem hiding this comment.
Can you expand this docstring to clarify the numeric range of each coordinate, and any constraints on the output, and also whether east is guaranteed to exceed 180 if the input crosses the antimeridian?
There was a problem hiding this comment.
I'll add a bit more info, but some of what you're asking for seems out of scope of a docstring.
| // Pass as MultiPoint to sidestep d3-geo's polygon winding assumption (CW | ||
| // exterior, opposite of RFC 7946) and geodesic apex overshoot |
There was a problem hiding this comment.
I don't know what CW exterior or geodesic apex overshoot mean, could you tweak this comment to be a bit more readable?
There was a problem hiding this comment.
Yeah, that's confusing. I'll simplify it.
| return [ | ||
| west, | ||
| south, | ||
| east < west ? east + ANTIMERIDIAN_LON_SHIFT : east, // Unwrap antimeridian crossing; east may exceed 180° |
There was a problem hiding this comment.
Can use unwrapLonRange() here now that #7881 is merged, right?
There was a problem hiding this comment.
This check is slightly different, but I can probably get everything to use the same calc. I'll update it.
| @@ -54,17 +54,25 @@ function calcGeoJSON(calcTrace, fullLayout) { | |||
| calcPt._polygons = geoUtils.feature2polygons(feature); | |||
|
|
|||
| var bboxFeature = geoUtils.computeBbox(feature); | |||
There was a problem hiding this comment.
I realize this issue predates your changes, but it seems a little silly to compute bounding boxes for all the features here if they might be ignored in the next step. Maybe a small refactor would make sense?
It's beyond the scope of this PR though so not necessary.
There was a problem hiding this comment.
Good idea! That does seem a bit wasteful. Let me poke at it.
| } | ||
|
|
||
| describe('Test geo fitbounds with antimeridian-straddling points', function() { | ||
| describe('Test geo fitbounds with antimeridian-straddling points', function () { |
There was a problem hiding this comment.
Is it worth adding a similar test here for choropleth to verify the changes end-to-end? And/or an image test? (I'm a little surprised that none of the existing image baselines changed as a result of these changes, but I guess it requires a pretty specific combination of factors to surface this issue.)
There was a problem hiding this comment.
In the follow up work to default to fitbounds = "locations", I'll address adding tests. This one just needed to be updated because of computeBbox changed.
emilykl
left a comment
emilykl
left a comment
There was a problem hiding this comment.
Left some nitpick comments, but LGTM.
2 participants
Description
Update
computeBboxto handle geojson features that cross the antimeridian.Closes #4436.
Changes
computeBboxcall sitesTesting
Screenshots
Notes
@turf/bboxdoesn't handle geometry that spans the antimeridian and there's been no movement to fix that issued3-geo.geoBounds, that does work with the antimeridian) to do the same thing (along with a turf utility used within @turf/bbox)